Repository navigation
fix(providerplatforms): allowlist order_by in OrderClause (EN-229) - #4
Conversation
OrderClause returned a bare order_by when no direction was sent and coerced a bad direction to desc. It now takes the columns a query may sort by and falls back to the query's default for anything else, matching core and programs.
carddev81
left a comment
carddev81
left a comment
There was a problem hiding this comment.
@CK-7vn , the code here looks good. OrderClause now matches core, getOutcomesForUser is the only caller and passes created_at and type, and the courses and catalog sorts were already safe through validOrder and the field map. The new tests cover them anyway. go vet and go test ./... pass at 944d05b.
One thing to fix:
The PR description is garbled. Parts of it got cut off or run together, for example "OrderClause(fallback string, all/providerplatforms/http.go", "andesc", "passes crenger pre-resets" and "http_test.go catalog sorts`". There's also an empty "Description of the change" heading at the top and a second one further down. Please rewrite it so a reviewer can read it.
While you're in there, change "matching core and programs": the Programs fix isn't in any open PR right now (see my note on core #1294). Also, outcomes with no order_by now default to outcomes.created_at DESC instead of the old created_at ascending. Nothing in the frontend calls this endpoint, so it's fine, but it's worth a line in the description.
EN-229 can't close from this PR alone. It also needs core #1294 and a new unlocked-programs PR merged (details are on #1294).
Description of the change
OrderClausereturned a bareorder_bywhen no direction was sent and coerced a bad direction todesc, but it never checked the column. It now takes the columns a query may sort by and falls back to the query's default for anything else, matching core'sOrderClause(UnlockedLabs/unlocked-core#1294).queryContext.OrderClause(fallback string, allowed ...string)inhttp.goreturns<order_by> <dir>only whenorder_byis inallowedand the direction isascordesc(case-insensitive;ascwhen absent). Anything else returnsfallback.getOutcomesForUser(store.go) passescreated_atandtypeas its allowlist, replacing the check that rewroteargs.OrderBy.order_clause_test.gocovers the direction and allowlist cases, hostileorder_by/orderon outcomes, and the user courses and catalog sorts (already safe throughvalidOrderand the field map; characterization tests). The old coerce-to-descassertion inhttp_test.gois removed, since a bad direction now falls back instead.Behavior change: outcomes with no
order_bynow default tooutcomes.created_at DESCinstead ofcreated_atascending. Nothing in the frontend calls this endpoint.EN-229 also needs UnlockedLabs/unlocked-core#1294 and UnlockedLabs/unlocked-programs#5.
Pre-Submission PR Checklist
No debug/console/fmt.Println statements
Unnecessary development comments removed
All acceptance criteria verified
Functions according to ticket specifications
Tested manually where applicable (covered by automated tests only; no manual or Postgres run)
Branch rebased with latest main
No business logic exists within the database layer
DB migrations (if any) are backward-compatible and online-safe (no migrations)
PR title and every commit are conventional commits (
feat:,fix:,docs:...); a squash merge lands under the PR title, and both become the changelogRelated issues: EN-229
Release
fix